ci: add audit B4 benchmark visual and shader gates - #888

kcov coverage

Line coverage ran for this PR and uploaded a non-blocking report artifact named kcov-report. Codecov upload is configured as non-blocking while the project captures a stable baseline.

📋 Summary

PR #888 closes #873, #874, #875, and #876. It wires benchmark triggers/Bencher publishing, adds absolute SLO gates to the benchmark harness, converts visual-test to a deterministic golden-image RMSE diff, and gates SPIR-V size regressions in zig build test. The implementation largely satisfies the acceptance criteria, but a previously reported security finding remains unresolved.

🔴 Critical Issues (Must Fix - Blocks Merge)

[CRITICAL] .github/workflows/benchmark.yml:127 - Unresolved curl | bash security pattern
Confidence: High
Description: The "Publish Bencher trends" step still installs the Bencher CLI via curl ... https://bencher.dev/download/install-cli.sh | sh. This was flagged by github-advanced-security in the previous review and has not been fixed in this revision.
Impact: If Bencher's CDN or domain is compromised, arbitrary code executes on the CI runner with access to BENCHER_API_TOKEN, BENCHER_PROJECT, and the repository context.
Suggested Fix: Replace the pipe with a pinned download + checksum verification. For example, download the install script to a temporary file, verify a SHA-256 checksum against a committed known-good hash, then execute it; or use a pinned GitHub release binary downloaded with curl and verified before execution.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] scripts/check_spirv_sizes.sh:35-38 - Test run mutates committed baseline
Confidence: Medium
Description: When a new shader appears, the script rewrites docs/shaders/spirv-sizes.json in place. zig build test should be read-only with respect to committed baselines; silently updating baselines during test execution makes CI non-reproducible and can mask unreviewed shader bloat.
Impact: CI may pass while leaving a dirty working tree, and future baselines can inherit regressions without explicit review.
Suggested Fix: Separate "update baseline" into a distinct maintenance command (e.g., scripts/update_spirv_baseline.sh), and have check_spirv_sizes.sh fail when a new shader is missing from the baseline with a clear message telling the user to run the update script.

[MEDIUM] modules/game-core/src/benchmark.zig:251-253 / src/game/app.zig:534-543 - GPU memory SLO checks configured budget, not measured usage
Confidence: Medium
Description: The gpu_memory_budget_mb SLO compares the preset's hardcoded budget value against a hardcoded ceiling. Because app.zig always sets the budget to exactly the ceiling for the preset, this check is effectively a tautology and cannot detect a real GPU memory overshoot.
Impact: The "GPU memory budget" gate cannot catch actual memory leaks or excessive allocations; it only validates that two constant tables agree.
Suggested Fix: Either measure actual GPU memory via the RHI/Vulkan backend and compare against the preset ceiling, or rename this SLO to "configured GPU memory budget" and document that it is a configuration guard until runtime memory tracking is added.

ℹ️ Low Priority Suggestions (Optional)

[LOW] scripts/check_spirv_sizes.sh:24 - date +%s%N is Linux-specific
Confidence: Low
Description: The compile-time timing relies on GNU date +%s%N, which is not portable to macOS or BSD.
Impact: Local development on non-Linux platforms may fail or behave unexpectedly.
Suggested Fix: Use EPOCHREALTIME or a portable fallback when date +%s%N is unavailable.

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 40%

Confidence Breakdown:

• Code Quality: 60% (Zig code is clean; shell scripts are mostly robust but have a security flaw.)
• Completeness: 75% (Acceptance criteria for the four linked issues are broadly met.)
• Risk Level: 30% (The unresolved curl | bash pattern is a supply-chain risk.)
• Test Coverage: 60% (SLOs and visual diff are wired; the SPIR-V baseline mutation weakens test integrity.)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The functional changes are solid and the linked issues are mostly addressed, but the unresolved curl | bash security finding must be fixed before merging.

{
  "reviewed_sha": "8df046e53baada24f296b267c3bcf8ba95463eb0",
  "critical_issues": 1,
  "high_priority_issues": 0,
  "medium_priority_issues": 2,
  "overall_confidence_score": 40,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

📋 Summary

PR #888 closes #873, #874, #875, and #876. It wires benchmark triggers/Bencher publishing, adds absolute SLO gates to the benchmark harness, converts visual-test to a deterministic golden-image RMSE diff, and gates SPIR-V size regressions in zig build test. The implementation broadly satisfies the acceptance criteria for all four linked issues.

📌 Review Metadata

• Reviewed Commit SHA: e6588b9dd834ddd19b52426473fd66991fd09a26
• Reviewed PR: ci: add audit B4 benchmark visual and shader gates #888

Linked issues: #873, #874, #875, #876 — all are addressed in the current changeset.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

✅ [FIXED] Previous critical issue: The curl | bash Bencher CLI install pattern in the benchmark workflow has been replaced with the bencherdev/bencher@main action, resolving the Semgrep OSS supply-chain finding.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] .github/workflows/benchmark.yml:141 - Unpinned third-party Bencher action
Confidence: Medium
Description: The Install Bencher CLI step uses bencherdev/bencher@main. Using a mutable tag for a third-party action is a supply-chain risk; a compromised or broken release can execute in the CI runner with access to BENCHER_API_TOKEN.
Impact: A malicious or accidentally breaking release of the Bencher action can leak the API token, alter benchmark results, or break CI.
Suggested Fix: Pin the action to a full-length commit SHA, e.g., bencherdev/bencher@<sha>, and update it explicitly via Dependabot or a similar mechanism.

[MEDIUM] scripts/check_spirv_sizes.sh:35-42 - Test run mutates committed baseline
Confidence: Medium
Description: When a new shader is found, the script rewrites docs/shaders/spirv-sizes.json in place. zig build test should be read-only with respect to committed baselines.
Impact: CI may pass while leaving a dirty working tree, and future baselines can inherit regressions without explicit review.
Suggested Fix: Separate "update baseline" into a distinct maintenance script (e.g., scripts/update_spirv_baseline.sh), and have check_spirv_sizes.sh fail when a new shader is missing from the baseline with a clear message telling the user to run the update script.

[MEDIUM] modules/game-core/src/benchmark.zig:251-253 / src/game/app.zig:537-543 - GPU memory SLO checks configured budget, not measured usage
Confidence: Medium
Description: The gpu_memory_budget_mb SLO compares the preset's hardcoded budget value against a hardcoded ceiling. Because app.zig always sets the budget to exactly the ceiling for the preset, this check is effectively a tautology.
Impact: The "GPU memory budget" gate cannot catch actual memory leaks or excessive allocations; it only validates that two constant tables agree.
Suggested Fix: Either measure actual GPU memory via the RHI/Vulkan backend and compare against the preset ceiling, or rename this SLO to "configured GPU memory budget" and document that it is a configuration guard until runtime memory tracking is added.

ℹ️ Low Priority Suggestions (Optional)

[LOW] scripts/check_spirv_sizes.sh:24 - date +%s%N is Linux-specific
Confidence: Low
Description: The compile-time timing relies on GNU date +%s%N, which is not portable to macOS or BSD.
Impact: Local development on non-Linux platforms may fail or behave unexpectedly.
Suggested Fix: Use EPOCHREALTIME or a portable fallback when date +%s%N is unavailable.

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 70%

Confidence Breakdown:

• Code Quality: 75% (Zig code is clean; shell scripts are mostly robust but have baseline/test-integrity issues.)
• Completeness: 85% (Acceptance criteria for the four linked issues are broadly met.)
• Risk Level: 60% (The unpinned Bencher action and SLO tautology remain low-grade risks.)
• Test Coverage: 75% (SLOs and visual diff are wired; the SPIR-V baseline mutation weakens test integrity.)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The functional changes are solid and the linked issues are addressed, but the unpinned action and test-integrity/SLO issues should be fixed or explicitly accepted before merging.

{
  "reviewed_sha": "e6588b9dd834ddd19b52426473fd66991fd09a26",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 3,
  "overall_confidence_score": 70,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

📋 Summary

PR #888 closes #873, #874, #875, and #876. It implements PR benchmark triggering, Bencher publishing, absolute benchmark SLO gates, deterministic golden-image visual regression, and a SPIR-V size regression gate. The implementation satisfies all four linked issues.

📌 Review Metadata

• Reviewed Commit SHA: 4759d3d5076749f02ae72fb6ff2ec22be02d2309
• Reviewed PR: ci: add audit B4 benchmark visual and shader gates #888

Linked issues: #873, #874, #875, #876 — all are addressed in the current changeset.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

The earlier curl | bash Bencher CLI install pattern was replaced with a pinned bencherdev/bencher action in .github/workflows/benchmark.yml:124.

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] .github/workflows/visual-test.yml:139,150 - Unpinned third-party anomalyco/opencode/github@latest action
Confidence: Medium
Description: Both opencode helper steps reference anomalyco/opencode/github@latest. A mutable tag for a third-party action is a supply-chain risk; a compromised release can execute in the CI runner with access to OPENCODE_PAT and MINIMAX_API_KEY.
Impact: A malicious or broken release can exfiltrate secrets, tamper with visual diagnostics, or alter failure analysis.
Suggested Fix: Pin the action to a full-length commit SHA, e.g., anomalyco/opencode/github@<sha>, and update it explicitly via Dependabot or a similar mechanism.

ℹ️ Low Priority Suggestions (Optional)

[LOW] scripts/check_spirv_sizes.sh:25,28 - Linux-specific utilities
Confidence: Low
Description: The script uses date +%s%N for timing and stat -c '%s' for file size. Both are GNU/Linux-specific and will break or misbehave on macOS/BSD.
Impact: Local development on non-Linux platforms may fail when running zig build test.
Suggested Fix: Use a portable timer such as EPOCHREALTIME or perl -MTime::HiRes, and a portable size command such as wc -c < "$output" or ls -l with a cross-platform parser.

[LOW] scripts/check_spirv_sizes.sh:55 - Baseline accumulates stale entries
Confidence: Low
Description: The script only iterates over existing shader files. If a shader is removed from the repository, its entry remains in docs/shaders/spirv-sizes.json.
Impact: The committed baseline can grow stale, making future diffs or audits slightly misleading.
Suggested Fix: After the loop, warn or exit if the baseline contains shader paths not present in the current working tree.

[LOW] scripts/compare_visual_golden.sh:24 - magick compare errors are masked
Confidence: Low
Description: The || true fallback means if magick compare fails for a reason other than exceeding the RMSE threshold (e.g., image dimension mismatch), the actual error message is discarded and the script reports a generic RMSE failure.
Impact: Debugging visual-test failures is harder because the root cause is lost.
Suggested Fix: Capture the exit code separately; if the exit code is non-zero and the RMSE value could not be parsed, print the raw magick compare output and exit with a distinct error.

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

• Code Quality: 80% (Zig code is clean; shell scripts are mostly robust.)
• Completeness: 90% (Acceptance criteria for the four linked issues are fully met.)
• Risk Level: 65% (The unpinned opencode action is a supply-chain risk, but lower than the prior curl | bash issue.)
• Test Coverage: 80% (SLOs, visual diff, and SPIR-V baseline gates are all wired.)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The functional changes are solid and all linked issues are addressed, but the unpinned third-party opencode action should be pinned to a SHA before merging.

{
  "reviewed_sha": "4759d3d5076749f02ae72fb6ff2ec22be02d2309",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 75,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

📋 Summary

PR #888 closes #873, #874, #875, and #876. It implements benchmark triggers/Bencher publishing, absolute benchmark SLO gates, deterministic golden-image visual regression, and a SPIR-V size regression gate. All four linked issues are addressed in the current changeset.

📌 Review Metadata

• Reviewed Commit SHA: f2d80f90e7fbd6601f7c99befc5c45d1d019ffa5
• Reviewed PR: ci: add audit B4 benchmark visual and shader gates #888

Linked issues: #873, #874, #875, #876 — all are addressed in the current changeset.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

The earlier curl | bash Bencher CLI install pattern in .github/workflows/benchmark.yml has been replaced with the pinned bencherdev/bencher@30a740a2e4246560b1a5fd424057d84aa2b188d6 action.

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] .github/workflows/benchmark.yml:81-82 - Missing pull-requests: write permission for PR comment step
Confidence: Medium
Description: The permissions block grants issues: write but only pull-requests: read. The "Comment Bencher dashboard" step calls github.rest.issues.createComment on a pull request, which typically requires pull-requests: write in GitHub Actions.
Impact: The Bencher dashboard comment may fail with a 403 permission error on pull requests, even though the rest of the workflow succeeds.
Suggested Fix: Add pull-requests: write to the job permissions, or replace the unnecessary issues: write with pull-requests: write.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous medium issue: The unpinned bencherdev/bencher and anomalyco/opencode/github actions are now pinned to full-length commit SHAs across all workflows.

✅ [FIXED] Previous medium issue: scripts/check_spirv_sizes.sh no longer silently rewrites docs/shaders/spirv-sizes.json during zig build test; it requires SPIRV_UPDATE_BASELINE=1 and fails with a clear message when a new shader is missing.

✅ [FIXED] Previous medium issue: The GPU memory SLO now uses measured memory from rhi.device.getStats() rather than comparing two hardcoded configuration tables.

[LOW] scripts/check_spirv_sizes.sh:25,28 - Linux-specific utilities
Confidence: Low
Description: The script uses date +%s%N for timing and stat -c '%s' for file size. Both are GNU/Linux-specific and will break or misbehave on macOS/BSD.
Impact: Local development on non-Linux platforms may fail when running zig build test.
Suggested Fix: Use EPOCHREALTIME or a portable timer fallback, and wc -c < "$output" for size.

[LOW] scripts/check_spirv_sizes.sh:55 - Baseline accumulates stale entries
Confidence: Low
Description: The script only iterates over existing shader files. If a shader is removed from the repository, its entry remains in docs/shaders/spirv-sizes.json.
Impact: The committed baseline can grow stale, making future diffs or audits slightly misleading.
Suggested Fix: After the loop, warn or exit if the baseline contains shader paths not present in the current working tree.

[LOW] scripts/compare_visual_golden.sh:24 - magick compare errors are masked
Confidence: Low
Description: The || true fallback means if magick compare fails for a reason other than exceeding the RMSE threshold (e.g., image dimension mismatch), the actual error message is discarded and the script reports a generic RMSE failure.
Impact: Debugging visual-test failures is harder because the root cause is lost.
Suggested Fix: Capture the exit code separately; if the exit code is non-zero and the RMSE value could not be parsed, print the raw magick compare output and exit with a distinct error.

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 80%

Confidence Breakdown:

• Code Quality: 85% (Zig code is clean; shell scripts are mostly robust; memory tracking is now measured.)
• Completeness: 90% (Acceptance criteria for the four linked issues are fully met.)
• Risk Level: 75% (Permissions misconfiguration and minor portability issues are low-grade risks.)
• Test Coverage: 85% (SLOs, visual diff, and SPIR-V baseline gates are all wired and now read-only.)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The functional changes are solid and all linked issues are addressed, but add pull-requests: write to the benchmark workflow so the Bencher dashboard comment can actually post.

{
  "reviewed_sha": "f2d80f90e7fbd6601f7c99befc5c45d1d019ffa5",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 80,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

📋 Summary

PR #888 closes #873, #874, #875, and #876. It implements PR benchmark triggering and Bencher publishing, absolute benchmark SLO gates, deterministic golden-image visual regression, and a SPIR-V size regression gate. All four linked issues are addressed in the current changeset.

📌 Review Metadata

• Reviewed Commit SHA: 695cc04a3f228998f5302db2074a27daeb75760f
• Reviewed PR: ci: add audit B4 benchmark visual and shader gates #888

Linked issues: #873, #874, #875, #876 — all are addressed in the current changeset.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

The earlier curl | bash Bencher CLI install pattern in .github/workflows/benchmark.yml has been replaced with the pinned bencherdev/bencher@30a740a2e4246560b1a5fd424057d84aa2b188d6 action.

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

None identified.

✅ [FIXED] Previous medium issue: The missing pull-requests: write permission in .github/workflows/benchmark.yml:34 has been added, allowing the Bencher dashboard comment to post on pull requests.

ℹ️ Low Priority Suggestions (Optional)

[LOW] scripts/check_spirv_sizes.sh:25,28 - Linux-specific utilities
Confidence: Low
Description: The script uses date +%s%N for timing and stat -c '%s' for file size. Both are GNU/Linux-specific and will break or misbehave on macOS/BSD.
Impact: Local development on non-Linux platforms may fail when running zig build test.
Suggested Fix: Use EPOCHREALTIME or a portable timer fallback, and wc -c < "$output" for size.

[LOW] scripts/check_spirv_sizes.sh:55 - Baseline accumulates stale entries
Confidence: Low
Description: The script only iterates over existing shader files. If a shader is removed from the repository, its entry remains in docs/shaders/spirv-sizes.json.
Impact: The committed baseline can grow stale, making future diffs or audits slightly misleading.
Suggested Fix: After the loop, warn or exit if the baseline contains shader paths not present in the current working tree.

[LOW] scripts/compare_visual_golden.sh:24 - magick compare errors are masked
Confidence: Low
Description: The || true fallback means if magick compare fails for a reason other than exceeding the RMSE threshold (e.g., image dimension mismatch), the actual error message is discarded and the script reports a generic RMSE failure.
Impact: Debugging visual-test failures is harder because the root cause is lost.
Suggested Fix: Capture the exit code separately; if the exit code is non-zero and the RMSE value could not be parsed, print the raw magick compare output and exit with a distinct error.

📊 SOLID Principles Score

🎯 Final Assessment

Overall Confidence Score: 85%

Confidence Breakdown:

• Code Quality: 85% (Zig code is clean; shell scripts are mostly robust; memory tracking is now measured.)
• Completeness: 90% (Acceptance criteria for the four linked issues are fully met.)
• Risk Level: 80% (All previously reported security findings are resolved; only minor portability and diagnostic issues remain.)
• Test Coverage: 85% (SLOs, visual diff, and SPIR-V baseline gates are all wired and read-only.)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE — All previously reported blocking and medium-priority issues have been resolved, and the remaining suggestions are low-priority portability and diagnostic improvements.

{
  "reviewed_sha": "695cc04a3f228998f5302db2074a27daeb75760f",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 85,
  "recommendation": "MERGE"
}

opencode session  |  github run